Minor code clean-ups and performance improvements #906

Thorium · 2024-03-15T10:00:49Z

Code clean-up with minor performance optimizations:

isNull instead of "= null": https://latkin.org/blog/2015/05/18/null-checking-considerations-in-f-its-harder-than-you-think/
TryGetValue instead of double-lookups of dictionaries
Types and active patterns that are not containing any data can be structs to reduce object capsuling and memory footprint
inline functions preferably internal to not leak outside of the project

No functional changes.

nojaf

Hi @Thorium, this is quite insightful.

It sparks some interest to detect these things via analyzers.
Before I dwindle in that territory, did you detect these manually? Or use some tooling?

Last question: is any of this noticeable? We don't have a benchmark so I'm not asking you to add that but did you run this before and after? Just curious.

nojaf · 2024-03-15T10:41:28Z

src/Common/StringParsing.fs

@@ -15,8 +15,12 @@ open FSharp.Formatting.Markdown

 module String =
    /// Matches when a string is a whitespace or null
+    [<return: Struct>]


Do you have any idea under which conditions you would start using this?

What is the minimum language version?

Always for unit or can this be used for other return types as well?

Struct constructs came with FSharp.Core 6.0.1 so that's the minimum version.
Works with other return types too.

nojaf · 2024-03-15T10:42:25Z

src/Common/StringParsing.fs


 module List =
    /// Matches a list if it starts with a sub-list that is delimited
    /// using the specified delimiters. Returns a wrapped list and the rest.
-    let inline (|DelimitedWith|_|) startl endl input =
+    let inline internal (|DelimitedWith|_|) startl endl input =


So internal because this is used in other files in the project?
Otherwise private would also work?

I just want to limit the scope to this solution: As you know inline makes compiler slower. So if you have hundreds of public inline functions in your solution and someone refers your project via Nuget package, they are going to pay the price with their compilation too, because those inline-functions come available for them too, even when they probably are not aware of them and probably will never need them.

This is my reasoning only, and I might be wrong:
If do code like this: myInt |> and now you ask your F# IDE to auto-complete, then when it knows the type of "myInt" then it can give proper function options: Those who take int in this case.

But in case of inline functions, they use statically resolved type parameters.

So if you have a referenced 10s other nuget packages and all have 10s of inline functions, then... all of those might be potentials for your auto-complete. You probably don't want call those, so it's just noise that could be avoided if they'd be internal functions.

nojaf · 2024-03-15T10:45:16Z

src/FSharp.Formatting.Common/YaafFSharpScripting.fs


 open ArgParser

+[<Struct>]


The heuristic would be here to add [<Struct>] when the DU only has cases without any fields?

Yes, Enums and similar are struct per default, so if you do this:

type MyE = | A = 1 | B = 2

it's fine as is. But if you do this:

type MyE = | A | B

...it's not a struct. So basically it'll generate a class of MyE containing a class of A and class of B and an instance of either.

Thorium · 2024-03-15T11:23:51Z

FSharpLint can detect the null-check, but it's a problematic project because it seems to break on each new major .NET release (which are often) and it randomly crashes on many other cases. The others were manual.

Thorium · 2024-03-15T11:56:08Z

General guidelines about structs (and I don't have good references to point here):

Keep your structs small.

By small I mean a bool or guid or int or int64 or another struct, but not bunch of other classes or interface-implementations of any kind or big lists or anything like that.

Struct is read to memory at once and kept there, so it's not a pointer to somewhere else, and it makes the processing fast and footprint small. The limitation is function parameters, structs are copied and not sent as pointers, which means if you have big structs, then function calls go slow. Which you can avoid by making your structs as ByRef and so on, but then they are mutable and it goes as hack-hack rather than proper enterprise code. But that's why e.g. System.Text.Json is so fast, they use big structs byref and trust to take 100% out of your single core.

If you have a simple object having a few non-class fields, then it's better to be struct. If you have a 1000s of these small objects it will really improve the performance and memory footprint when processing them through.

Option class is not a struct, but there is a struct-corresponding ValueOption. ValueOption has IsSome and IsNone and typically the usage with match-clause will just work. And there is ValueOption.bind and so-on like Option.bind you've used to. But if you use lot of existing F# functions like List.tryPick they work with Some x and not ValueSome x, and there I'd go for usability over performance.

Tuple class is not a struct but, but there is a ValueTuple which is used instead of (a,b) like this: struct(a,b). That's a bit inconvenient to use.

So this is still good:

[<Struct>]
type MyType =
| MyOp of int
| MyOp2 of bool ValueOption 
| MyOp3 of struct(bool*bool)

They also say you should performance-test, to verify your changes, because many times it depends on use-case if a harmless looking [<Struct>] type MyType = MyOp of int array is better to be a struct or not.

So for conservative analysis, I'd go for only non-data-carrying and/or only plain boolean/int/guid/struct-field types to be recommended to be structs.

nojaf · 2024-03-15T11:59:31Z

Thanks for all the insight!

Last question: is any of this noticeable? We don't have a benchmark so I'm not asking you to add that but did you run this before and after? Just curious.

Thorium · 2024-03-15T12:04:29Z

src/FSharp.Formatting.Common/YaafFSharpScripting.fs

@@ -464,26 +463,29 @@ module internal ArgParser =
        else
            None

+    [<return: Struct>]


@nojaf here is an example where there is a Struct boolean. As boolean is a tiny field, there is no point of wrapping it to inside a separate class, like the original code did. Wrapping just makes the processing of comparisons etc slower.

Thorium · 2024-03-15T12:09:19Z

The comparisons are probably microseconds, so I expect it's not human noticeable in a single run of generating the docs, but if you have build-pipeline over every commit queuing generating docs, then even small changes can affect over time, to free up some CI workers faster.

nojaf · 2024-03-15T12:42:29Z

Sure, that is fair. I just wanted you to admit that you didn't check 😉.
A valuable PR nonetheless.

Thorium · 2024-04-01T07:16:34Z

Here is quite detailed reference post of Structs: https://www.bartoszsypytkowski.com/writing-high-performance-f-code/

Thorium added 2 commits March 15, 2024 09:55

Minor code clean-ups and performance improvements

495bc37

mandatory Fantomas formatting commit

c4f4e0d

nojaf approved these changes Mar 15, 2024

View reviewed changes

Thorium commented Mar 15, 2024

View reviewed changes

nojaf merged commit 1c4b2e0 into fsprojects:main Mar 15, 2024
3 checks passed

nojaf mentioned this pull request Mar 15, 2024

Add analyzers from FSharp.Formatting PR ionide/ionide-analyzers#85

Merged

7 tasks

Thorium mentioned this pull request Mar 15, 2024

Structs: DUs and active patterns that have no fields or only simple sturcts should also be structs fsprojects/FSharpLint#705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor code clean-ups and performance improvements #906

Minor code clean-ups and performance improvements #906

Thorium commented Mar 15, 2024

nojaf left a comment

nojaf Mar 15, 2024

Thorium Mar 15, 2024

nojaf Mar 15, 2024

Thorium Mar 15, 2024

Thorium Apr 1, 2024

nojaf Mar 15, 2024

Thorium Mar 15, 2024 •

edited

Loading

Thorium commented Mar 15, 2024

Thorium commented Mar 15, 2024 •

edited

Loading

nojaf commented Mar 15, 2024

Thorium Mar 15, 2024 •

edited

Loading

Thorium commented Mar 15, 2024

nojaf commented Mar 15, 2024

Thorium commented Apr 1, 2024

Minor code clean-ups and performance improvements #906

Minor code clean-ups and performance improvements #906

Conversation

Thorium commented Mar 15, 2024

nojaf left a comment

Choose a reason for hiding this comment

nojaf Mar 15, 2024

Choose a reason for hiding this comment

Thorium Mar 15, 2024

Choose a reason for hiding this comment

nojaf Mar 15, 2024

Choose a reason for hiding this comment

Thorium Mar 15, 2024

Choose a reason for hiding this comment

Thorium Apr 1, 2024

Choose a reason for hiding this comment

nojaf Mar 15, 2024

Choose a reason for hiding this comment

Thorium Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Thorium commented Mar 15, 2024

Thorium commented Mar 15, 2024 • edited Loading

nojaf commented Mar 15, 2024

Thorium Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Thorium commented Mar 15, 2024

nojaf commented Mar 15, 2024

Thorium commented Apr 1, 2024

Thorium Mar 15, 2024 •

edited

Loading

Thorium commented Mar 15, 2024 •

edited

Loading

Thorium Mar 15, 2024 •

edited

Loading